-
Notifications
You must be signed in to change notification settings - Fork 450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add echo-comments setting #1333
Conversation
52eeb11
to
41d9e08
Compare
I'm not 100% sure if "ignore-recipe-comments" is the best possible name for what this setting does. It's more like, don't echo comments within a recipe, but that's a bit unwieldy to make for a setting name. |
41d9e08
to
752e86b
Compare
src/parser.rs
Outdated
value: Setting::AllowDuplicateRecipes(value), | ||
name, | ||
}); | ||
let set_bool: Option<Setting> = if Keyword::AllowDuplicateRecipes == lexeme { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice refactor, imo, but I think you could take it one more step if you wanted and turn this into a match
expression instead of a series of if/else
.
Just looking around out of curiosity and going to throw my 2 cents in (feel free to take it or leave it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally thought it wasn't possible to do this because lexeme
is &str
and can't be directly pattern-matched against Keyword
, but I think if I parse the lexeme
first I can pattern-match against that and make this bit of code a bit more concise. Thanks for the suggestion!
752e86b
to
09ba6d3
Compare
What about |
Wondering if maybe |
I thought about that, but I think that hard-coding the notion of a Justfile comment does make sense. There's basically two types of recipes, shebang and linewise. A linewise recipe is lines of shell code, and In a shebang recipe, the entire recipe block is treated as source code in some other language, and
where
Currently fails at the node level rather than at the just level, because This PR as written only applies to linewise recipes and not shebang ones (maybe it would make sense to clarify that in the definition of the setting), so it basically establishes that a |
The only issue there is that it means that the option would have to default to true in order to retain the current behavior in existing justfiles. All the other boolean options are default false. Is that a principled decision or just a coincidence? |
Sorry for letting this sit! My concern is that someone might use But thinking about it, it seems like having a simple setting that does the right thing 99.9% of the time is probably a good idea, and if someone complains that their justfile looks weird, we can cross that bridge when we come to it.
It's kind of a principled decision, since I think it's nice when settings are default off, but if we don't have a better name, I'm open to breaking it. I think |
src/line.rs
Outdated
match self.fragments.first() { | ||
Some(Fragment::Text { token }) => { | ||
let lexeme = token.lexeme(); | ||
lexeme.starts_with("#") && !lexeme.starts_with("#!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only called for linewise recipes, then does it need to check for #!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, but the code can't guarantee that no one will ever try to call is_comment
in a shebang recipe in the future.
On the other hand, maybe it makes sense to say that, once you're in the body of a linewise recipe, #!
should be interpreted as a comment that happens to start with a !
rather than a shebang. Actually I think this is the correct thing to do, because right now just only checks to see if the first line in a recipe is a #!
, not any other ones. So if the code encounters a #!
after the first line, it should treat it as a comment that happens to begin with !
, not as a shebang.
93e0743
to
f474991
Compare
How about Edit: Thinking about this again, |
f474991
to
3c4e59b
Compare
Btw @casey do you know why https://github.com/casey/just/actions/runs/3120617565/jobs/5061331667 is failing? It looks like this is because of clippy errors, but when I run stable clippy locally it reports nothing wrong. |
@neunenak Sounds good, let's go with Try with rust 1.56, which is what CI uses: |
3c4e59b
to
7b3c201
Compare
7b3c201
to
c3c72f4
Compare
Nice, looks good to me! |
Whoops! I actually found something weird. Line continuations in comments cause the next line to be included in the comment, so this test fails:
This is a bit odd, since normally the contents of a comment, including backslashes, are ignored. What do you think? This also made me think of potentially better name for this setting |
And sorry for letting this sit! Next time hit the request review button or add a comment letting me know it's ready to take a look at. |
No worries. I'm actually not sure where in the Github UI the request review button is (I've been using Gitlab at work so I'm much more used to their UI these days). As I recall, Github had a notion of Draft vs Ready open PRs, but I'm not sure where a "ready to review" button would live, unless that's a project-specific thing. |
Huh, this is weird, and not something I expected - probably has something to do with how the tokenizer works? I would definitely expect that test case to succeed, I'll try to fix it. |
Actually, thinking about this a bit, I think this is a bug (or at least unintuitive behavior) with how just comments work in general. Ignoring this change, on the current published version of just, a justfile:
will print the following:
and not print "HELLO". This is definitely unintuitive, but it's also how just comments currently work, and it's the sort of thing that would definitely break existing justfiles if it changed suddenly. Deciding what to do about this deserves it's own issue (I'd suggest adding this to the list of things to fix in just edition 2). I don't think it needs to block this PR though, since echoing or not echoing what |
So I think the issue is that, in recipe bodies, just doesn't have comments at all. It doesn't do anything differently if a line starts with This change makes just treat those lines as comments, so any aspect of how they behave can be changed, and it wouldn't be a breaking change, since it's opt-in with the setting. I think it's reasonable to have this setting just kind of turn on comments in general, i.e. doesn't echo them, and also considers them to be a single line, and ignores line continuations at the end of them. |
c8db42a
to
7ff93b5
Compare
Makes sense. This actually turned out to be pretty easy to fix to make that test case pass - just add |
7ff93b5
to
17d736b
Compare
Add a new setting "echo-comments", which defaults to true. If unset, this causes lines internal to a non-shebang recipe beginning with the character '#' (including '#!' internal to a non-shebang recipe; that is, any such instances occurring after the first line of a recipe) to be treated as comments of the justfile itself. They will not be echoed to stderr when the recipe executes.
17d736b
to
2b6bf7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, merged! I decided to change the name back to ignore-comments
. I think it's the clearest. Thanks for sticking with this! I expect that this is something that we'll want to default to true in a future edition.
Add a new setting "ignore-recipe-comments". If set, this causes lines internal to a non-shebang recipe beginning with the character '#' (other than '#!' itself) to be treated as comments of the justfile itself. They will not be echoed to stderr when the recipe executes.
Can potentially close #1274